Tunnel sush through Nexus - #11254
Conversation
This completes the initial integration of the [Support Shell](https://github.com/oxidecomputer/sush) ([RFD 620](https://rfd.shared.oxide.computer/rfd/0620)). We add an unpublished, fleet-admit-only WebSocket endpoint to Nexus that tunnels bytes to a sush proxy in a switch zone. Nexus controls only who may reach the proxy; the platform-ID backed sprockets-TLS, client authn, and job authz inside are untouched. The tunnel is the first WebSocket endpoint that refuses in HTTP before upgrading, so the authz coverage harness gained a method for it. The harness expects privileged requests to fail with 503, because no sush proxy runs in the test environment. The bad-authn probes include the handshake headers, so they reach the authn check and fail with its status. Co-Authored-By: Claude Mythos 5 <noreply@anthropic.com>
0ce2c9d to
7dc9f6d
Compare
990fb83 to
b6aa684
Compare
| /// Authorize the tunnel and connect to a sush proxy, on whichever | ||
| /// switch answers first. |
There was a problem hiding this comment.
This says "whichever answers first", but it looks like we try the switches one-at-a-time with a timeout.
Did you want to parallelize this and drop one? or is this intentionally serialized?
There was a problem hiding this comment.
Good catch, thank you! Actually raced in a2906e4.
| "thiserror 2.0.18", | ||
| "tokio", | ||
| "tokio-postgres", | ||
| "tokio-tungstenite 0.23.1", |
There was a problem hiding this comment.
Is this the version used by dropshot? As a PSA it's a couple years old
There was a problem hiding this comment.
This was the current workspace version, used by MGS and its serial console, but not by Nexus's instance serial console, which is pinned to Propolis' version (0.21!). sush was already on 0.28, so bumped to that in 34a9b52 to get us back down to two versions.
| switch_zone_address_mappings(&self.internal_resolver, log) | ||
| .await | ||
| .map_err(|e| Error::unavail(&e))? | ||
| .into_values() | ||
| .map(|ip| { | ||
| SocketAddr::V6(SocketAddrV6::new(ip, SUSH_PROXY_PORT, 0, 0)) | ||
| }) | ||
| .collect::<Vec<_>>(); |
There was a problem hiding this comment.
This may be an unnecessarily expensive way of looking up the switch zone IPs, since you're dropping the switch slot anyway (with the call to into_values).
switch_zone_address_mappings has two parts:
- Look up the switch zone address of Dendrite via DNS (you want this)
- For each switch zone, create an MGS client, contact it asking for information (I don't think you want this)
This seems like it unnecessarily couples the support shell proxy to the availability of MGS, which is a bummer. Could we just use the lookup_all_ipv6 part for Dendrite and drop the rest?
There was a problem hiding this comment.
Thank you very much, applied in 188ae1b.
| ) -> WebsocketEndpointResult { | ||
| let apictx = rqctx.context(); | ||
| let nexus = &apictx.context.nexus; | ||
| let path = path_params.into_inner(); |
There was a problem hiding this comment.
WDYT about wrapping this in an audit_and_time call, so that the audit log can see it? Logging access to the support shell seems like a good idea.
There was a problem hiding this comment.
That sounds good to me. It would be the only GET endpoint we audit (seems like it has to be a GET, otherwise I would propose changing it to POST), so it needs special treatment in the coverage test in nexus/tests/integration_tests/audit_log.rs:
- Explicitly expect auditing for this endpoint despite its GET method.
- Send WebSocket handshake headers so the request reaches authorization. Currently it would fail before it gets to the audit log call, potentially concealing missing audit coverage.
There was a problem hiding this comment.
Agreed, thank you. Added with test coverage in 1767f5d.
| } | ||
| to_proxy += data.len() as u64; | ||
| } | ||
| Ok(Message::Close(_)) | Err(_) => break, |
There was a problem hiding this comment.
I get why the Ok(Message::Close(_)) case terminates quietly, but shouldn't a read error (the Err(_) case) fail loudly?
There was a problem hiding this comment.
Indeed it should, fixed in 352c0f8 along with reads from the proxy, thanks.
| /// If we fail to resolve the ipv6 addresses of the Dendrite service we | ||
| /// return an error | ||
| async fn switch_zone_address_mappings( | ||
| pub(crate) async fn switch_zone_address_mappings( |
There was a problem hiding this comment.
See my comment below about why calling this method might be incorrect, but if you agree, we should make this private once more
There was a problem hiding this comment.
I do agree, reverted to private in 188ae1b.
| let log = match proxy.peer_addr() { | ||
| Ok(addr) => log.new(o!("proxy_addr" => addr)), | ||
| Err(_) => log, | ||
| }; |
There was a problem hiding this comment.
This call to proxy.peer_addr is fallible, but that's a little silly because inside support_shell_proxy we always know the address before returning the TcpStream result. Should we just return a tuple of (SocketAddr, TcpStream) from support_shell_proxy to avoid the dead-code error pathway here?
| "proxy closed" | ||
| }; | ||
|
|
||
| let reason = tokio::select! { |
There was a problem hiding this comment.
In the case where the inbound pipe hits the branch:
Ok(Message::Close(_)) | Err(_) => break,above, we'll proxy_write.shutdown() and exit this tokio::select.
But if that happens, the outbound async block, holding ws_sink, might not get a chance to close cleanly. We'll still drop ws_sink, so, maybe it doesn't matter, but I'm not sure that this will appear the same to the caller.
Should we call ws_sink.close().await after this tokio::select, to close more hygienically?
There was a problem hiding this comment.
Excellent suggestion, thank you. d8b0840 shuts down both sides after the select.
| /// Tunnel to a Support Shell proxy in a switch zone | ||
| // This should use `channel { protocol = WEBSOCKETS, .. }`, but | ||
| // that does not let us return (unauthorized) errors before the | ||
| // connection upgrade. |
There was a problem hiding this comment.
I was thrown off by the "should" — it makes it sound like this is something to fix in Dropshot. I found this alternative wording clearer, take it or leave it:
// Use #[endpoint] rather than #[channel] so this handler can
// authorize the request and connect to the proxy before upgrading
// to WebSocket. With #[channel], Dropshot upgrades the connection
// before calling the handler, making it too late to return HTTP errors.
Co-Authored-By: Claude Mythos 5 <noreply@anthropic.com>
Co-Authored-By: Claude Mythos 5 <noreply@anthropic.com>
Co-Authored-By: Claude Mythos 5 <noreply@anthropic.com>
Co-Authored-By: Claude Mythos 5 <noreply@anthropic.com>
Co-Authored-By: Claude Mythos 5 <noreply@anthropic.com>
Co-Authored-By: Claude Mythos 5 <noreply@anthropic.com>
Co-Authored-By: Claude Mythos 5 <noreply@anthropic.com>
76fc529 to
1767f5d
Compare
Co-Authored-By: Claude Mythos 5 <noreply@anthropic.com>
Co-Authored-By: Claude Mythos 5 <noreply@anthropic.com>
Co-Authored-By: Claude Mythos 5 <noreply@anthropic.com>
Co-Authored-By: Claude Mythos 5 <noreply@anthropic.com>
Message::Binary now takes Bytes rather than Vec<u8>. Co-Authored-By: Claude Mythos 5 <noreply@anthropic.com>
d650a9a to
d615d83
Compare
Co-Authored-By: Claude Mythos 5 <noreply@anthropic.com>
1dad322 to
746e8ce
Compare
This completes the initial integration of the Support Shell (RFD 620), building on #11252 and #11253.
We add an unpublished, fleet-admit-only WebSocket endpoint to Nexus that tunnels bytes to a sush proxy in a switch zone. Nexus controls only who may reach the proxy; the platform-ID backed sprockets-TLS, client authn, and job authz inside are untouched.
The tunnel is the first WebSocket endpoint that refuses in HTTP before upgrading, so the authz coverage harness gained a method for it. The harness expects privileged requests to fail with 503, because no sush proxy runs in the test environment. The bad-authn probes include the handshake headers, so they reach the authn check and fail with its status.